Skip to content

Class name is rule id #212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Class name is rule id #212

wants to merge 2 commits into from

Conversation

arielkr256
Copy link
Contributor

@arielkr256 arielkr256 commented Mar 13, 2025 •
edited
Loading

Description

This PR updates how rule id attributes are handled in the pypanther framework. Previously, the framework required explicit setting of the id attribute and would fail validation if it was missing. Now, the id is automatically set to the class name in init_subclass if not explicitly defined.

Key changes:

Modified init_subclass to set id to the class name if not explicitly defined
Updated validation logic to reflect this new behavior
Updated test cases to verify the automatic id assignment
This change reduces boilerplate code by eliminating the need to explicitly set id = "ClassName" in every rule class, while maintaining backward compatibility with rules that do explicitly set their IDs.

References

N/A

Checklist

[x] Added unit tests
Updated test_rule_missing_id to test_rule_id_is_class_name to verify automatic ID assignment
Test verifies that id is correctly set to class name when not explicitly defined
[x] Tested end to end
Verified existing rules continue to work with both explicit and implicit IDs

@arielkr256 arielkr256 requested a review from a team as a code owner March 13, 2025 20:41
@arielkr256 arielkr256 marked this pull request as draft March 13, 2025 20:45
@arielkr256
Copy link
Contributor Author

Class names are not guaranteed to be unique, which is a requirement for rule ID - hold for now.

@jacknagz
Copy link
Contributor

cc @kostaspap

@kostaspap
Copy link
Contributor

kostaspap commented Mar 27, 2025 •
edited
Loading

cc @kostaspap

Some thoughts on this: I don't think id should be optional. Either it should be required, or should not be there at all. I don't think we want want second-guessing what's the ID of a rule.

If we were to use the class name as rule ID, some things that come to mind:

  1. To enforce uniqueness we either need to include the module path in the id of the rule (which is not so nice -lots of repetition, a simple refactoring in code changes the rule IDs) or force "globally" unique class names.
  2. Class names don't allow fancy characters. No spaces, dots, etc. That means that likely the pattern of rule id most customers (and we internally) use have to change
  3. Following up to the above, we have to think how this will impact the migration path from v1 to v2. Now we suggest customers to just set the V2 to have the same rule id as V1 and v2 "wins". We would have to come up with a new approach.
  4. We should consider how migration path for customers already running pypanther rules will look like, especially considering they might not be able to keep the same ids for their existing rules.

All in all.... I'm trying to think whether the root issue can be addressed in a different way. What's the problem we are trying to address with this change and are there any alternatives? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arielkr256 @jacknagz @kostaspap